properly handle revision based_on field

Author: escattone

kitsune/wiki/handlers.py

@@ -30,6 +30,24 @@ def on_user_deletion(self, user: User) -> None:
         Revision.objects.filter(readied_for_localization_by=user).update(
             readied_for_localization_by=sumo_bot
         )
+        # The based-on value of localized revisions is essential, so if the based-on revision
+        # is un-approved and created by the user to be deleted, we should ensure that it is
+        # retained by re-assigning it to the "sumo_bot". It's very rare that the based-on
+        # revision of a localized revision is un-approved (less than 1%), but it is possible.
+        # The based-on value of non-localized revisions is not nearly as important, so in those
+        # cases, we can simply let the cascade behavior set the based-on value to NULL.
+        Revision.objects.filter(
+            id__in=(
+                Revision.objects.exclude(document__locale=settings.WIKI_DEFAULT_LANGUAGE)
+                .filter(
+                    based_on__creator=user,
+                    based_on__is_approved=False,
+                    # This locale check ensures only legitimate based-on references.
+                    based_on__document__locale=settings.WIKI_DEFAULT_LANGUAGE,
+                )
+                .values_list("based_on__id", flat=True)
+            )
+        ).update(creator=sumo_bot)
 
         documents = Document.objects.filter(contributors=user)
         for document in documents:

kitsune/wiki/migrations/0019_alter_revision_based_on.py

@@ -0,0 +1,24 @@
+# Generated by Django 4.2.20 on 2025-03-21 08:59
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("wiki", "0018_alter_document_restrict_to_groups"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="revision",
+            name="based_on",
+            field=models.ForeignKey(
+                blank=True,
+                null=True,
+                on_delete=django.db.models.deletion.SET_NULL,
+                to="wiki.revision",
+            ),
+        ),
+    ]

kitsune/wiki/models.py

@@ -836,7 +836,7 @@ class Revision(ModelBase, AbstractRevision):
     # Edit button was hit to begin creating this revision. If there was none,
     # this is simply the latest of the default locale's revs as of that time.
     # Used to determine whether localizations are out of date.
-    based_on = models.ForeignKey("self", on_delete=models.CASCADE, null=True, blank=True)
+    based_on = models.ForeignKey("self", on_delete=models.SET_NULL, null=True, blank=True)
     # TODO: limit_choices_to={'document__locale':
     # settings.WIKI_DEFAULT_LANGUAGE} is a start but not sufficient.
 
@@ -965,7 +965,6 @@ def latest_revision(excluded_rev, constraint):
             except IndexError:
                 return None
 
-        Revision.objects.filter(based_on=self).update(based_on=None)
         document = self.document
 
         # If the current_revision is being deleted, try to update it to the

kitsune/wiki/tests/test_handlers.py

@@ -205,3 +205,68 @@ def test_mixed_revisions_user_deletion(self):
 
         rev2 = Revision.objects.filter(id=rev2_id).first()
         self.assertIsNone(rev2, "Deleted user's revision should be deleted")
+
+    def test_revision_based_on_deletion(self):
+        """
+        Test the handling of a revision's "based_on" field when it references a revision
+        created by a user that is deleted.
+        """
+        doc = DocumentFactory()
+        rev1 = RevisionFactory(document=doc, creator=self.user)
+        rev2 = RevisionFactory(document=doc, based_on=rev1)
+        rev3 = ApprovedRevisionFactory(document=doc, based_on=rev2)
+
+        doc_id = doc.id
+        rev1_id = rev1.id
+        rev2_id = rev2.id
+        rev3_id = rev3.id
+
+        self.listener.on_user_deletion(self.user)
+
+        self.assertTrue(
+            Document.objects.filter(id=doc_id).exists(), "Document should not be deleted"
+        )
+        self.assertFalse(
+            Revision.objects.filter(id=rev1_id).exists(),
+            "Deleted user's unapproved revision should be deleted",
+        )
+        rev2 = Revision.objects.filter(id=rev2_id).first()
+        self.assertIsNotNone(rev2, "Other user's revision should not be deleted")
+        self.assertIsNone(rev2.based_on, "Other user's revision's based_on should now be None")
+        rev3 = Revision.objects.filter(id=rev3_id).first()
+        self.assertIsNotNone(rev3, "The approved revision should not be deleted")
+        self.assertEqual(rev3.based_on, rev2)
+
+        it_doc = DocumentFactory(locale="it", parent=doc)
+        it_rev = RevisionFactory(document=it_doc, based_on=rev2)
+
+        it_doc_id = it_doc.id
+        it_rev_id = it_rev.id
+
+        self.listener.on_user_deletion(rev2.creator)
+
+        # Ensure that the default document wasn't deleted.
+        self.assertTrue(
+            Document.objects.filter(id=doc_id).exists(), "Document should not be deleted"
+        )
+
+        # The deleted user's un-approved revision should not have been deleted,
+        # but instead re-assigned to the sumo bot, because it's the based-on
+        # value of a localized revision.
+        rev2 = Revision.objects.filter(id=rev2_id).first()
+        self.assertIsNotNone(rev2, "Deleted user's revision should not be deleted")
+        self.assertEqual(rev2.creator.username, settings.SUMO_BOT_USERNAME)
+
+        # Ensure that the third revision, whose based-on value is the revision
+        # whose creator was deleted, was not affected at all.
+        rev3 = Revision.objects.filter(id=rev3_id).first()
+        self.assertIsNotNone(rev3, "The approved revision should not be deleted")
+        self.assertEqual(rev3.based_on, rev2)
+
+        self.assertTrue(
+            Document.objects.filter(id=it_doc_id).exists(),
+            "The Italian document should not be deleted",
+        )
+        it_rev = Revision.objects.filter(id=it_rev_id).first()
+        self.assertIsNotNone(it_rev, "The Italian revision should not be deleted")
+        self.assertEqual(it_rev.based_on, rev2)